-
Notifications
You must be signed in to change notification settings - Fork 790
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Graphql plugin - temporary here for reviews - will be moved to contrib repo later #1573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1573 +/- ##
=======================================
Coverage 92.93% 92.93%
=======================================
Files 156 156
Lines 4871 4871
Branches 988 988
=======================================
Hits 4527 4527
Misses 344 344 |
@open-telemetry/javascript-approvers ^^ |
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
// is moving to instrumentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1572 has been merged so this need to be updated
private _addPatchingExecute(): InstrumentationNodeModuleFile< | ||
typeof graphqlTypes | ||
> { | ||
const file = new InstrumentationNodeModuleFile<typeof graphqlTypes>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we just return the newly created class instead of assigning it ?
'graphql/execution/execute.js', | ||
// cannot make it work with appropriate type as execute function has 2 | ||
//types and/cannot import function but only types | ||
(moduleExports: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use unknown
here and check manuallly that the wrapped functions exist ?
document: graphqlTypes.DocumentNode, | ||
rootValue: any, | ||
contextValue: any, | ||
variableValues: Maybe<{ [key: string]: any }>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be Maybe<Record<string, any>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to be the same as in graphql
*/ | ||
|
||
export * from './graphql'; | ||
export * from './symbols'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why should we export the symbols ? If thats just for the tests, could we just directly import them ?
* The maximum depth of fields/resolvers to instrument. | ||
* When set to 0 it will not instrument fields and resolvers | ||
* | ||
* @default undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the default -1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it is fine
closing in favour of open-telemetry/opentelemetry-js-contrib#226 |
@vmarchaud deleted my comment but it seems I have deleted yours accidently :/ |
I'm pushing this here as instrumentation has not been published yet, once it is done I'm happy to move it to contrib, but for easier review this can be reviewed here too.